Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made Paintable::DrawMode an enum class #13424

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

JoergAtGithub
Copy link
Member

No description provided.

@github-actions github-actions bot added the ui label Jun 30, 2024
@@ -16,7 +16,7 @@ class QSvgRenderer;
// high fidelity.
class Paintable {
public:
enum DrawMode {
enum class DrawMode {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this change results in a touch to every point of use anyways, can you also change the members to TitleCase?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed, that the upper case strings are used accross the skin XML files. Therefore the string must remain upper case for backward compatibility.
PS: I just added a debug assert, which worked;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have to decide, should we switch to Q_ENUM or should we rename to TitelCase?
Since Q_ENUM adds a overhead, I would prefer to rename and implement these two functions otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Q_ENUM adds a overhead

Out of curiosity, in what sense? From what I can tell, it only adds a little bit of compilation time due to the MOC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the class must be a QObject and the MOC compilation comes on top.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The QObject could be avoided if the enum was moved to a namespace technically. Though I'm fine with leaving it as a plain enum class for now. Its unlikely that this enum would need to get exposed to the QML environment anyways (which is where the biggest value of a QEnum lies IMO).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enum is only used in our legacy widgets. A use with QML is highly unlikely.

@JoergAtGithub JoergAtGithub requested a review from Swiftb0y July 1, 2024 21:55
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@Swiftb0y Swiftb0y merged commit c0b4f50 into mixxxdj:main Jul 2, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants